Adding class option to broker create #1402

vyasgun · 2021-07-27T15:16:13Z

Description

Adding --class option to broker create

Changes

Added the flag to broker create command
Added unit test
Added e2e test

Reference

Fixes #1269

knative-prow-robot

@vyasgun: 0 warnings.

In response to this:

Description

Adding --class option to broker create

Changes

Added the flag to broker create command

Added unit test

Added e2e test

Reference

Fixes #1269

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2021-07-27T15:20:13Z

Codecov Report

Merging #1402 (a4a6d5f) into main (a252d9b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1402      +/-   ##
==========================================
+ Coverage   78.33%   78.35%   +0.02%     
==========================================
  Files         160      160              
  Lines        8270     8279       +9     
==========================================
+ Hits         6478     6487       +9     
  Misses       1103     1103              
  Partials      689      689

Impacted Files	Coverage Δ
pkg/eventing/v1/client.go	`82.67% <100.00%> (+0.85%)`	⬆️
pkg/kn/commands/broker/create.go	`71.42% <100.00%> (+3.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a252d9b...a4a6d5f. Read the comment docs.

itsmurugappan

/lgtm

pkg/kn/commands/broker/create.go

maximilien

Left some comments. Thanks for contributions

maximilien · 2021-07-28T23:41:22Z

lib/test/broker.go

@@ -32,6 +32,13 @@ func BrokerCreate(r *KnRunResultCollector, name string) {
 	assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "Broker", name, "created", "namespace", r.KnTest().Kn().Namespace()))
 }

+// BrokerCreateWithClass creates a broker with the given name and class.


Why is these not named CreateBroker…?

The convention in this file is BrokerCreate, BrokerDelete etc so I am just following that.

The convention was already established in the file, not change by this PR. I propose to keep it as it and if we want to change it do it in a separate move.

maximilien · 2021-07-28T23:43:19Z

test/e2e/broker_test.go

@@ -59,6 +59,12 @@ func TestBroker(t *testing.T) {
 	test.BrokerDelete(r, "foo4", true)
 	verifyBrokerNotfound(r, "foo3")
 	verifyBrokerNotfound(r, "foo4")
+
+	t.Log("create broker with class")
+	test.BrokerCreateWithClass(r, "foo5", "foo-class")


Are there naming conventions for the class that we should be verifying… e.g., cannot start with numbers or symbols?

There seems to be no server side validation for this so I am not sure.

dsimansk · 2021-07-29T11:08:51Z

Please re-run ./hack/build.sh -c.

knative-metrics-robot · 2021-07-29T12:05:04Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/eventing/v1/client.go	87.7%	88.3%	0.6
pkg/kn/commands/broker/create.go	83.3%	85.0%	1.7

rhuss · 2021-07-30T08:04:00Z

pkg/kn/commands/broker/create.go

+  # Create a broker 'mybroker' in the 'myproject' namespace and with a broker class of 'Kafka'
+  kn broker create mybroker --namespace myproject --broker Kafka


Suggested change

# Create a broker 'mybroker' in the 'myproject' namespace and with a broker class of 'Kafka'

kn broker create mybroker --namespace myproject --broker Kafka

# Create a broker 'mybroker' in the 'myproject' namespace and with a broker class of 'KafkaBroker'

kn broker create mybroker --namespace myproject --broker KafkaBroker

Sorry my bad again, I think it should be KafkaBroker (so with a trailing Broker suffix.

I am following the KNative Kafka Broker documentation and it is just Kafka as per it:

apiVersion: eventing.knative.dev/v1 kind: Broker metadata: annotations: # case-sensitive eventing.knative.dev/broker.class: Kafka name: default namespace: default spec: # Configuration specific to this broker. config: apiVersion: v1 kind: ConfigMap name: kafka-broker-config namespace: knative-eventing # Optional dead letter sink, you can specify either: # - deadLetterSink.ref, which is a reference to a Callable # - deadLetterSink.uri, which is an absolute URI to a Callable (It can potentially be out of the Kubernetes cluster) delivery: deadLetterSink: ref: apiVersion: serving.knative.dev/v1 kind: Service name: dlq-service

Ok, I was referring to https://knative.dev/docs/eventing/broker/example-mtbroker/ where it used as KafkaBroker. @matzew which is correct now ?

I think it's just Kafka because I tried creating 2 brokers with the different classes and the Kafka controller only picked up when the annotation was set to Kafka and not when it was set to KafkaBroker but we can confirm this once before we decide to push.

Let me open an issue over there at the docs repo, to find out and get confirmation from the experts ;-)

I just asked over there to clarify this knative/docs#4090 (would really prefer "Kafka" but then also "MTChannel" instead of "MTChannelBroker" ;-)

@rhuss I think the current example message is correct. Any objections to proceed and merge this PR?

+1 to merge it.

rhuss · 2021-08-06T13:05:22Z

/approve
/lgtm

knative-prow-robot · 2021-08-06T13:05:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: itsmurugappan, rhuss, vyasgun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 27, 2021

knative-prow-robot reviewed Jul 27, 2021

View reviewed changes

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 27, 2021

vyasgun force-pushed the pr/broker-class branch from 1d7c799 to e064f08 Compare July 27, 2021 15:17

vyasgun force-pushed the pr/broker-class branch from e064f08 to a201885 Compare July 27, 2021 15:28

Adding --class flag to broker create command

b623d82

vyasgun force-pushed the pr/broker-class branch from a201885 to b623d82 Compare July 27, 2021 15:35

itsmurugappan approved these changes Jul 28, 2021

View reviewed changes

knative-prow-robot assigned itsmurugappan Jul 28, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2021

rhuss reviewed Jul 28, 2021

View reviewed changes

pkg/kn/commands/broker/create.go Outdated Show resolved Hide resolved

Updated broker class usage

f374815

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2021

rhuss reviewed Jul 28, 2021

View reviewed changes

pkg/kn/commands/broker/create.go Show resolved Hide resolved

rhuss reviewed Jul 28, 2021

View reviewed changes

pkg/kn/commands/broker/create.go Show resolved Hide resolved

maximilien suggested changes Jul 28, 2021

View reviewed changes

knative-prow-robot assigned maximilien Jul 28, 2021

vyasgun force-pushed the pr/broker-class branch from bc6d3ac to e077652 Compare July 29, 2021 07:24

Updated broker create examples

a4a6d5f

vyasgun force-pushed the pr/broker-class branch from e077652 to a4a6d5f Compare July 29, 2021 12:02

rhuss reviewed Jul 30, 2021

View reviewed changes

knative-prow-robot assigned rhuss Aug 6, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2021

knative-prow-robot merged commit 9d1bfc6 into knative:main Aug 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding class option to broker create #1402

Adding class option to broker create #1402

vyasgun commented Jul 27, 2021

knative-prow-robot left a comment

codecov bot commented Jul 27, 2021 •

edited

Loading

itsmurugappan left a comment

maximilien left a comment

maximilien Jul 28, 2021

vyasgun Jul 29, 2021

rhuss Jul 30, 2021

maximilien Jul 28, 2021

vyasgun Jul 29, 2021

dsimansk commented Jul 29, 2021

knative-metrics-robot commented Jul 29, 2021

rhuss Jul 30, 2021

vyasgun Jul 30, 2021

rhuss Jul 30, 2021

vyasgun Jul 30, 2021

rhuss Aug 2, 2021

rhuss Aug 2, 2021

dsimansk Aug 6, 2021

rhuss Aug 6, 2021

rhuss commented Aug 6, 2021

knative-prow-robot commented Aug 6, 2021

		# Create a broker 'mybroker' in the 'myproject' namespace and with a broker class of 'Kafka'
		kn broker create mybroker --namespace myproject --broker Kafka

Adding class option to broker create #1402

Adding class option to broker create #1402

Conversation

vyasgun commented Jul 27, 2021

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

codecov bot commented Jul 27, 2021 • edited Loading

Codecov Report

itsmurugappan left a comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Jul 29, 2021

knative-metrics-robot commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss commented Aug 6, 2021

knative-prow-robot commented Aug 6, 2021

codecov bot commented Jul 27, 2021 •

edited

Loading